Skip to content

feat: add a plugable logging system.#153

Open
yingcai-cy wants to merge 6 commits into
apache:mainfrom
yingcai-cy:logger
Open

feat: add a plugable logging system.#153
yingcai-cy wants to merge 6 commits into
apache:mainfrom
yingcai-cy:logger

Conversation

@yingcai-cy

Copy link
Copy Markdown
Contributor

No description provided.

Comment thread src/iceberg/util/logger.h Outdated
Comment thread src/iceberg/util/logger.h
Comment on lines +218 to +224
default_logger_ = std::static_pointer_cast<void>(logger);
log_func_ = [logger](LogLevel level, std::string_view format_str,
const std::string& formatted_message) {
if (logger->ShouldLog(level)) {
logger->LogImpl(level, "{}", formatted_message);
}
};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ultimately we still do a dynamic call, does it save us anything versus a virtual call? It is significantly more complicated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, function pointers are generally faster than virtual calls, and in some scenarios, the compiler is able to inline them, which can further improve performance.

Comment thread src/iceberg/util/logger.h Outdated
Comment thread src/iceberg/util/logger.h Outdated
Comment thread src/iceberg/util/logger.h Outdated
Comment thread src/iceberg/util/logger.h Outdated
Comment thread src/iceberg/util/logger.h Outdated
Comment thread src/iceberg/util/logger.h
Comment thread src/iceberg/util/logger.h Outdated
@evindj

evindj commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Hi folks, is this change ready for yet if not I am happy to take a stab at it as it is blocking the loggingmetricsreporter. let me know if there is more to do here, happy to have a look. @wgtmac @lidavidm

@wgtmac

wgtmac commented Jun 8, 2026

Copy link
Copy Markdown
Member

@evindj Thanks for your interest! I believe @kamcheungting-db is currently working on this so it is better to avoid conflict. We can postpone loggingmetricsreporter once logging has been added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants